-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Spelling errors #7456
Fix Spelling errors #7456
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
@@ -217,7 +217,7 @@ type Query struct { | |||
type Format int | |||
|
|||
const ( | |||
FloatFlormat Format = iota | |||
FloatFormat Format = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const FloatFormat should have comment (or a comment on this block) or be unexported
|
||
return tuple | ||
} | ||
|
||
func (t *TCPTuple) ComputeHashebles() { | ||
func (t *TCPTuple) ComputeHashables() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method TCPTuple.ComputeHashables should have comment or be unexported
|
||
return tuple | ||
} | ||
|
||
func (t *IPPortTuple) ComputeHashebles() { | ||
func (t *IPPortTuple) ComputeHashables() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method IPPortTuple.ComputeHashables should have comment or be unexported
@@ -66,7 +66,7 @@ const ( | |||
type matcher func(last, current []byte) bool | |||
|
|||
var ( | |||
sigMultilineTimeout = errors.New("multline timeout") | |||
sigMultilineTimeout = errors.New("multiline timeout") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error var sigMultilineTimeout should have name of the form errFoo
@@ -217,7 +217,7 @@ type Query struct { | |||
type Format int | |||
|
|||
const ( | |||
FloatFlormat Format = iota | |||
FloatFormat Format = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const FloatFormat should have comment (or a comment on this block) or be unexported
|
||
return tuple | ||
} | ||
|
||
func (t *TCPTuple) ComputeHashebles() { | ||
func (t *TCPTuple) ComputeHashables() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method TCPTuple.ComputeHashables should have comment or be unexported
|
||
return tuple | ||
} | ||
|
||
func (t *IPPortTuple) ComputeHashebles() { | ||
func (t *IPPortTuple) ComputeHashables() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported method IPPortTuple.ComputeHashables should have comment or be unexported
@@ -66,7 +66,7 @@ const ( | |||
type matcher func(last, current []byte) bool | |||
|
|||
var ( | |||
sigMultilineTimeout = errors.New("multline timeout") | |||
sigMultilineTimeout = errors.New("multiline timeout") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error var sigMultilineTimeout should have name of the form errFoo
@@ -1,5 +1,5 @@ | |||
{ | |||
"visState": "{\n \"title\": \"Event Actions [Auditbeat Auditd Overview]\",\n \"type\": \"metrics\",\n \"params\": {\n \"id\": \"61ca57f0-469d-11e7-af02-69e470af7417\",\n \"type\": \"timeseries\",\n \"series\": [\n {\n \"id\": \"61ca57f1-469d-11e7-af02-69e470af7417\",\n \"color\": \"#68BC00\",\n \"split_mode\": \"terms\",\n \"metrics\": [\n {\n \"id\": \"6b9fb2d0-c1bc-11e7-938f-ab0645b6c431\",\n \"type\": \"count\"\n }\n ],\n \"seperate_axis\": 0,\n \"axis_position\": \"right\",\n \"formatter\": \"number\",\n \"chart_type\": \"line\",\n \"line_width\": 1,\n \"point_size\": 1,\n \"fill\": 0.5,\n \"stacked\": \"none\",\n \"terms_field\": \"event.action\",\n \"label\": \"Actions\"\n }\n ],\n \"time_field\": \"@timestamp\",\n \"index_pattern\": \"auditbeat-*\",\n \"interval\": \"auto\",\n \"axis_position\": \"left\",\n \"axis_formatter\": \"number\",\n \"show_legend\": 1,\n \"show_grid\": 1,\n \"filter\": \"event.module:auditd\",\n \"background_color_rules\": [\n {\n \"id\": \"58c95a20-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"bar_color_rules\": [\n {\n \"id\": \"5bfc71a0-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"gauge_color_rules\": [\n {\n \"id\": \"5d20a650-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"gauge_width\": 10,\n \"gauge_inner_width\": 10,\n \"gauge_style\": \"half\",\n \"legend_position\": \"left\"\n },\n \"aggs\": []\n}", | |||
"visState": "{\n \"title\": \"Event Actions [Auditbeat Auditd Overview]\",\n \"type\": \"metrics\",\n \"params\": {\n \"id\": \"61ca57f0-469d-11e7-af02-69e470af7417\",\n \"type\": \"timeseries\",\n \"series\": [\n {\n \"id\": \"61ca57f1-469d-11e7-af02-69e470af7417\",\n \"color\": \"#68BC00\",\n \"split_mode\": \"terms\",\n \"metrics\": [\n {\n \"id\": \"6b9fb2d0-c1bc-11e7-938f-ab0645b6c431\",\n \"type\": \"count\"\n }\n ],\n \"separate_axis\": 0,\n \"axis_position\": \"right\",\n \"formatter\": \"number\",\n \"chart_type\": \"line\",\n \"line_width\": 1,\n \"point_size\": 1,\n \"fill\": 0.5,\n \"stacked\": \"none\",\n \"terms_field\": \"event.action\",\n \"label\": \"Actions\"\n }\n ],\n \"time_field\": \"@timestamp\",\n \"index_pattern\": \"auditbeat-*\",\n \"interval\": \"auto\",\n \"axis_position\": \"left\",\n \"axis_formatter\": \"number\",\n \"show_legend\": 1,\n \"show_grid\": 1,\n \"filter\": \"event.module:auditd\",\n \"background_color_rules\": [\n {\n \"id\": \"58c95a20-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"bar_color_rules\": [\n {\n \"id\": \"5bfc71a0-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"gauge_color_rules\": [\n {\n \"id\": \"5d20a650-c1bd-11e7-938f-ab0645b6c431\"\n }\n ],\n \"gauge_width\": 10,\n \"gauge_inner_width\": 10,\n \"gauge_style\": \"half\",\n \"legend_position\": \"left\"\n },\n \"aggs\": []\n}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kibana/5
and kibana/6
stuff probably should be dropped (I think my changes applied to kibana/7)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that they were backported to 6.something (that wasn't my expectation).
Initially, they should still be dropped here.
@@ -96,7 +96,7 @@ annotations: | |||
co.elastic.logs/multiline.pattern: '^\[' | |||
co.elastic.logs/multiline.negate: true | |||
co.elastic.logs/multiline.match: after | |||
co.elastic.logs.sidecar/exlude_lines: '^DBG' | |||
co.elastic.logs.sidecar/exclude_lines: '^DBG' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone should check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exekias Looks good I think.
@@ -208,7 +208,7 @@ | |||
} | |||
} | |||
}, | |||
"title": "Broswers breakdown [Filebeat IIS]", | |||
"title": "Browsers breakdown [Filebeat IIS]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is generated or source. This might need to be dropped (it may get dropped on its own unless it's specifically rescued when I try to drop the earlier kibana/5
/kibana/6
stuff)
@@ -29,7 +29,7 @@ import ( | |||
type endianness int8 | |||
|
|||
const ( | |||
unknownEndianess endianness = iota | |||
unknownEndianness endianness = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd. I can't find it outside this project, so I'm hoping I can make this change.
// determine endianess from BOM | ||
inEndiannes := unknownEndianess | ||
// determine endianness from BOM | ||
inEndianness := unknownEndianness |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a local variable
@@ -37,7 +37,7 @@ func TestNewGenerator(t *testing.T) { | |||
|
|||
v, _ := common.NewVersion("7.0.0") | |||
// checks for fields.yml | |||
generator, err := NewGenerator("beat-index", "mybeat.", filepath.Join(beatDir, "notexistent"), "7.0", *v) | |||
generator, err := NewGenerator("beat-index", "mybeat.", filepath.Join(beatDir, "nonexistent"), "7.0", *v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully not a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it will break the tests. We should also rename the directory.
@@ -58,17 +58,17 @@ type testOutputer struct { | |||
*esConnection | |||
} | |||
|
|||
type esSoure interface { | |||
type esSource interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this isn't a problem...
@@ -24,7 +24,7 @@ import ( | |||
"github.com/elastic/beats/libbeat/publisher/queue" | |||
) | |||
|
|||
type forgetfullProducer struct { | |||
type forgetfulProducer struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api change?
@@ -162,7 +162,7 @@ func getDomainMemoryStatName(tag int32) string { | |||
case 5: | |||
return "available" | |||
case 6: | |||
return "actualballon" | |||
return "actualballoon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaict this is the correct spelling of the kvm thing..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amandahla Can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the right spelling is "actualballoon"
packetbeat/protos/memcache/parse.go
Outdated
@@ -33,7 +33,7 @@ const ( | |||
type parserConfig struct { | |||
maxValues int | |||
maxBytesPerValue int | |||
parseUnkown bool | |||
parseUnknown bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect someone will ask for the indentation here to be fixed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That someone ended up being a build failure... (and painful adventures in go)
@jsoref What is the tool you used for this? Or is this all manual? |
@ruflin: corrections are picked by me manually. I've updated the PR with the tool. |
@@ -54,7 +54,7 @@ | |||
} | |||
], | |||
"point_size": 1, | |||
"seperate_axis": 0, | |||
"separate_axis": 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably not change things in the dashboards as these are based on an API. So the API might have a typo and we break it with these changes. This applies to everything under the _kibana
directory.
@@ -47,7 +47,7 @@ | |||
[2018-06-13T07:44:24.343+0000][32376][gc,heap ] GC(0) ParNew: 279616K->17562K(314560K) | |||
[2018-06-13T07:44:24.343+0000][32376][gc,heap ] GC(0) CMS: 0K->0K(699072K) | |||
[2018-06-13T07:44:24.343+0000][32376][gc,metaspace ] GC(0) Metaspace: 22819K->22819K(1071104K) | |||
[2018-06-13T07:44:24.343+0000][32376][gc ] GC(0) Pause Young (Allocation FailurGe) 273M->17M(989M) 13,344ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a log file from Elasticsearch and should not be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will drop
filebeat/prospector/prospector.go
Outdated
// Deprecated: See input.input | ||
type Prospectorer = input.Input | ||
type Prospector = input.Input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional and not a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Will drop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lot of good stuff in this PR. We should be very careful to not change any code / API bits even if their are typo as they might break things. I suggest we do not touch all the _kibana
files if possible.
Any chance you could split up this PR into mutiple parts? For example 1 per Beat? Like this we can get the changes in quicker and diffs are smaller.
We currently use missspell
tool do do something similar but it didn't chat lots of things which you scripted catched. Are there ways to potentially integrate something like this into CI? Any experience with this?
@jsoref What about creating something like hound that uses a webhook to do the spelling checks, I would love to have the spelling done inline as part of the PR review. Its more work for you but I know that you do this kind of PRs on multiple repositories. WDYT? :) |
Wow, thats awesome. Agree with @ruflin, we should split this up, so it becomes easier to review. From skimming the PR (brings my Browser to a crawl), the changes in the kibana dashboards make somewhat uneasy. Also the NOTICE.txt file is generated via I'd say, let's do a PR with go files (do not fix vendor) only. These are the most safest to clean up. No need to do it per beat. But some of the other files need some careful review. The formatting in the go code can be fixed using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the Kibana files, LGTM. Thanks!
dev-tools/.beatconfig
Outdated
@@ -1,4 +1,4 @@ | |||
packetbeat-/packetbeat- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone even know what this file is for? I think it can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming from #1439 and was linked to the old way of importing dashboard so it can be safely removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is that removals like that be done outside of a spelling PR. Before or after doesn't matter to me, but it's really problematic for people looking at a huge commit (and my understanding is that elastic tends to squash) to discover that in addition to doing a rather "simple" and "harmless" thing, it also did something else.
One positive side effect of spell checking is finding bugs and stale content.
That said, it's a personal preference. Within limits, I'll do as requested...
LGTM excepts the Kibana files. Thanks! @jsoref |
@ruflin: yes, I can split things. As long as I'm given guidance about how. My starting point is always per misspelling family because it makes dropping things (as you've highlighted a couple above) easier. If you suggest a directory split, I can do it. What I'll probably do is keep this as a tracking of the whole work and split off subsections into new PRs, updating this one dropping portions as they're merged in. |
@rufllin, @ph: I've created one Travis integration as noted in: jsoref/spelling#7 I'm unfamiliar with how hound works, but I'm not opposed. It won't be high on my to-do list. It took me over a year to get around to working on the Travis integration and probably about a month to get that working -- with a lot of help from the checkstyle/checkstyle project. If you offer similar assistance, I can certainly work on it. One thing to keep in mind: my tooling relies a lot on:
For CI, that last step is skipped, at best the tool can say:
The one downside of the whitelist, is it's essentially a file full of misspellings, which means every other spell checking person will need to ignore it. This isn't a big deal, realistically, no project wants to deal with a bot that can't take input/a human who acts so bot-like that it can't recognize it should ignore such a file. |
@urso: thanks, all Note: I often don't have access to complicated tool chains, but I think I've installed go because another project demanded it. (As is, I'm using a huge prob of a shared hosting system for this sort of work, installing every possible imaginable tool chain would cause my neighbors to be even more annoyed than they already are.) For the fmt thing, is it possible to have two PRs, a first to fix the fmt of the files and a second their spelling errors? As noted earlier, I find it very problematic to mix in different kinds of fixes into a single changeset. |
@urso: The reason there's no corresponding change for NOTICE is that I default to deleting For reference, that came from:
I may eventually send a PR to that module, but it isn't currently high on my list. |
3969e76
to
0a9f57c
Compare
Sorry. I can't get All I can say is that the
|
$AUSettigns = (New-Object -com "Microsoft.Update.AutoUpdate").Settings | ||
$AUSettigns.NotificationLevel = 1 | ||
$AUSettigns.Save() | ||
$AUSettings = (New-Object -com "Microsoft.Update.AutoUpdate").Settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewkroh Was this broken before or do these settings have a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaict, this is just a local variable
@@ -4,7 +4,7 @@ | |||
// that is unique to each beat. | |||
///// | |||
|
|||
[[seting-up-and-running]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we used this link knowwhere and is the reason this worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those things appear to be https://www.elastic.co/guide/en/beats/filebeat/current/seting-up-and-running.html#seting-up-and-running (and similar?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dedemorton Will this change break things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruflin There's a small chance that it could break the doc build (if there's another book that links to this topic), but it would be easy to fix.
The biggest impact is that the ID controls the html page file name, which means that I would need to get redirects set up for any branches that have this change.
I think it looks bad to have a typo in the URL, though, so I would go ahead with this change.
@@ -74,15 +74,15 @@ func eventMapping(content []byte) common.MapStr { | |||
"appendrequest": common.MapStr{ | |||
"count": data.Recv.Appendrequest.Count, | |||
}, | |||
"bandwithrate": data.Recv.Bandwithrate, | |||
"pkgrate": data.Recv.Pkgrate, | |||
"bandwidthrate": data.Recv.Bandwidthrate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news is etcd is still in beat so we can do these breaking changes. But we must add it to the changelog.
Left a few comments. One other thing we could do is to skip all the changes which we are not sure about and have them in a follow up PR. Like this we can keep 99% of the cahnges in this one. |
@jsoref Let us know if we can help somehow in moving this PR forward. There are so many good things in here I really want to get the "non critical" parts in and then do a follow up PR with all the discussion parts. |
|
@jsoref The commands you are looking for are |
jsoref-lightning:beats jsoref$ make update
New python executable in /Users/jsoref/code/elastic/beats/build/python-env/bin/python2.7
Also creating executable in /Users/jsoref/code/elastic/beats/build/python-env/bin/python
Installing setuptools, pip, wheel...done.
Generating NOTICE
Get the licenses available from ['./vendor', './metricbeat/vendor', './metricbeat/module/postgresql/vendor', './metricbeat/module/vsphere/vendor', './metricbeat/module/kvm/vendor', './metricbeat/module/mysql/vendor']
WARNING: No version information found for: github.com/stretchr/objx
Available at /Users/jsoref/code/elastic/beats/NOTICE.txt
New python executable in /Users/jsoref/code/elastic/beats/libbeat/build/python-env/bin/python2.7
Also creating executable in /Users/jsoref/code/elastic/beats/libbeat/build/python-env/bin/python
Installing setuptools, pip, wheel...done.
scripts/cmd/global_fields/main.go:26:2: cannot find package "github.com/elastic/beats/libbeat/generator/fields" in any of:
/usr/local/Cellar/go/1.10.3/libexec/src/github.com/elastic/beats/libbeat/generator/fields (from $GOROOT)
/Users/jsoref/go/src/github.com/elastic/beats/libbeat/generator/fields (from $GOPATH)
make[2]: *** [fields] Error 1
make[1]: *** [libbeat_fields] Error 2
make: *** [update] Error 1 |
jenkins, test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tests go green, this is ready to go. I also add a note about the breaking change to the Changelog.
@jsoref Thanks for all your work on this one. It does not only fix typos but in some cases even fix tests which were not run properly before. As a follow up we should probably continue the discussion how we could even automate this in the future.
@jsoref Merged. THANK YOU. |
@jsoref Thank you! I really appreciate the work you did here. |
@ruflin Adding a spell checker to the doc build process has been on my wish list for awhile (see elastic/docs#249 (comment)). It would be great to find a way to automate this across the docs (at least, the files in master). |
@dedemorton as I noted earlier, it should be possible to adapt https://github.com/checkstyle/checkstyle/blob/master/.ci/test-spelling-unknown-words.sh ... At some point, I'll go around and do it for a bunch of projects... |
@jsoref I was talking specifically about what we need to do to our current doc tool chain so that spell checking is baked into our builds, but it's spectacular that you've taken this work on! great stuff. |
@ruflin Are you planning to backport this? I want to make sure I set up any redirects that are required. |
@dedemorton No backports to 6.3 but will end up in 6.4. |
Generated by https://github.com/jsoref/spelling
f
; to maintain your repo, please considerfchurn